Skip to content

Fix unpairing IterableDatasetDict preference datasets - #6924

Closed
DaoyuanLi2816 wants to merge 1 commit into
huggingface:mainfrom
DaoyuanLi2816:fix/unpair-iterable-dataset-dict
Closed

Fix unpairing IterableDatasetDict preference datasets#6924
DaoyuanLi2816 wants to merge 1 commit into
huggingface:mainfrom
DaoyuanLi2816:fix/unpair-iterable-dataset-dict

Conversation

@DaoyuanLi2816

@DaoyuanLi2816 DaoyuanLi2816 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #6877.

unpair_preference_dataset handled DatasetDict separately but sent IterableDatasetDict.column_names, a split-to-columns mapping, directly to remove_columns. The iterable map therefore retained chosen and rejected, whose original batch length conflicts with the doubled unpaired output.

This change reads the column names from the first split for both dataset-dictionary types and adds regression coverage that materializes the unpaired iterable split.

Validation:

  • 10 passed for TestUnpairPreferenceDataset
  • focused Ruff check and format check
  • git diff --check

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline, Pull Request section?
  • Was this discussed/approved via a GitHub issue? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

AI writing disclosure

We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.

  • No AI usage: the PR was written entirely by a human.
  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
  • AI-generated: the PR was mostly or fully generated by an AI tool.

Who can review?

Anyone in the community is free to review the PR once the tests have passed.


Note

Low Risk
Small, targeted fix in preference dataset preprocessing with regression tests; no auth or security impact.

Overview
Fixes #6877 by correcting how unpair_preference_dataset resolves columns for IterableDatasetDict.

Previously only DatasetDict took column names from the first split; IterableDatasetDict fell through to the iterable path and passed column_names (a per-split dict) into remove_columns, so chosen / rejected were not dropped and batched unpairing could misalign lengths. The function now treats DatasetDict and IterableDatasetDict the same: column_names comes from next(iter(dataset.values())).column_names.

Adds test_unpair_preference_iterable_dataset_dict to assert an iterable split unpairs to the expected rows when materialized.

Reviewed by Cursor Bugbot for commit 90c8397. Bugbot is set up for automated code reviews on this repo. Configure here.

@DaoyuanLi2816

Copy link
Copy Markdown
Contributor Author

Closing in favor of #6878, which predates this PR and covers the same #6877 fix more completely, including the schema-less IterableDatasetDict case.

@behroozazarkhalili

Copy link
Copy Markdown
Collaborator

Your diagnosis is right and the one-liner is the correct fix. I checked out the branch and ran it both ways rather than reading the diff, and along the way found a second copy of the same bug twenty lines down that this PR does not fix.

Confirming yours first. On main, list(unpair_preference_dataset(idd)["t"]) raises IndexError: list index out of range, which is worth putting in the PR text since it is the string someone will search for. The cause is what you say: IterableDatasetDict.column_names is {'t': ['prompt', 'chosen', 'rejected']}, and that dict is truthy, so the or list(next(iter(dataset)).keys()) fallback never runs and remove_columns receives it. With your change the same call returns 4 rows from 2 prompts with completion, label, prompt. Reverting only the data_utils.py hunk and keeping your test gives 1 failed, 9 passed, and the one failure is your new test with that same IndexError. Your widened isinstance also matches what _get_dataset_format already does at data_utils.py:671.

The second copy is maybe_unpair_preference_dataset at data_utils.py:547:

if isinstance(dataset, DatasetDict):
    column_names = dataset[list(dataset.keys())[0]].column_names
else:
    column_names = dataset.column_names        # IterableDatasetDict -> the same dict
if "chosen" in column_names and "rejected" in column_names:

For an IterableDatasetDict that membership test looks at the dict's keys, which are split names, so it is False. Run on your branch:

'chosen' in column_names -> False
result rows = [{'prompt': 'a', 'chosen': 'x', 'rejected': 'p'},
               {'prompt': 'b', 'chosen': 'y', 'rejected': 'q'}]
n_rows = 2   (unchanged; 4 would mean unpaired)

It returns the paired dataset and says nothing, which is a worse failure than the one you fixed because there is no signal at all. The function is public, exported at trl/__init__.py:43, but the only in-tree caller is the experimental BCO trainer, which does not look like it accepts iterable datasets, so today you would reach it by calling the function directly.

Both functions sit next to each other and share one root cause, so I would fix them together here rather than in a second PR. Your call, and I would not block on it.

The test builds its expected rows by zipping to_dict().values(). Four literal dicts would be easier to check by eye, and would not quietly follow along if unpaired_dataset ever changes.

Thanks for catching something that only shows up on the streaming path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: unpair IterableDatasetDict preference datasets

2 participants